BDMS-430 Migrate legacy FieldParameters chemistry table#394
Conversation
… to ChemistrySampleInfo
ksmuczynski
requested review from
chasetmartin,
Copilot,
jirhiker and
kbighorse
There was a problem hiding this comment.
Pull request overview
This PR implements migration infrastructure for legacy FieldParameters chemistry data by adding an ORM model, Alembic migration, comprehensive tests, and a transfer script with foreign key validation.
Changes:
- Added
NMAFieldParametersmodel with proper foreign key relationships toChemistrySampleInfo - Created Alembic migration to build the legacy table structure with indexes
- Implemented transfer script with batch upsert, FK validation, and orphan prevention
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| transfers/field_parameters_transfer.py | Transfer script with batch upsert logic and ChemistrySampleInfo FK validation |
| tests/test_field_parameters_legacy.py | Comprehensive CRUD and relational integrity tests for the new model |
| db/nma_legacy.py | NMAFieldParameters model with relationship to ChemistrySampleInfo |
| alembic/versions/c1d2e3f4a5b6_create_nma_field_parameters.py | Migration to create legacy table structure with indexes |
chasetmartin
left a comment
chasetmartin
left a comment
There was a problem hiding this comment.
This is looking great! The two copilot comments do seem relevant.
Migration ran ok for me, and tests passed. I think the one missing item is updating the transfer.py file so that that we can run and test the transfer of field parameters. I would look to this Stratigraphy PR for how to update transfer.py:
https://github.com/DataIntegrationGroup/OcotilloAPI/pull/393/changes#diff-d2d33d2e5666e926931898df688d5d66a04e7d8148800dec2399b1de947e1fbf
Remove space in `Chemistry SampleInfoFieldParameters` index Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Remove space in `Chemistry SampleInforFieldParameters` index name Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…on' into kas-nma-field-parameters-migration
The view mirrors the legacy table’s columns, search/sort behavior, and keeps the records read-only like other legacy datasets.
| assert record.sample_pt_id == sample_info.sample_pt_id | ||
| assert record.field_parameter is None | ||
| assert record.units is None | ||
| assert record.sample_value == 0 |
There was a problem hiding this comment.
The assertion expects sample_value to default to 0, but the test creates a record without explicitly setting this field. This test is verifying the server_default behavior from the model/migration. However, SQLAlchemy ORM won't populate server defaults until after a commit and refresh. The assertion should occur after session.refresh(record) to ensure the server default is loaded from the database.
Why
This PR addresses the following problem / context:
How
Implementation summary - the following was changed / added / removed:
Notes
Any special considerations, workarounds, or follow-up work to note?